Move validation classes, make them non-static and adapt tests #37
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR relocates the iCalendar validation classes into a new package, converts them from singletons to instantiable classes, and updates all consumers and tests to use injected instances.
- Moved validation classes under
at.bitfire.synctools.icalendar.validation - Changed
objectpreprocessors to classes and updatedICalPreprocessor/ICalendarParserfor instance-based usage - Adapted tests to instantiate and inject preprocessors via MockK
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ICalPreprocessor.kt | Converted from object to class, added constructor usage and @VisibleForTesting for stream preprocessors |
| StreamPreprocessor.kt, FixInvalidUtcOffsetPreprocessor.kt, FixInvalidDayOffsetPreprocessor.kt | Updated packages and made preprocessors instantiable |
| ICalendarParser.kt | Added constructor injection for ICalPreprocessor and switched static calls to instance calls |
Test files under lib/src/test/kotlin/.../validation/ |
Updated imports, instantiated classes, and rewrote MockK usage |
| Task.kt, ICalendar.kt, Event.kt | Updated KDoc references to new validation package |
Comments suppressed due to low confidence (5)
lib/src/test/kotlin/at/bitfire/synctools/icalendar/validation/FixInvalidUtcOffsetPreprocessorTest.kt:19
- [nitpick] Test function names use underscores and PascalCase segments; consider using lowerCamelCase (e.g.,
testFixStringNoOccurrence) to align with Kotlin conventions.
fun test_FixString_NoOccurrence() {
lib/src/test/kotlin/at/bitfire/synctools/icalendar/validation/FixInvalidUtcOffsetPreprocessorTest.kt:10
- assertTrue is used in this test but not imported; add
import org.junit.Assert.assertTrueto fix compilation.
import org.junit.Assert.assertFalse
lib/src/test/kotlin/at/bitfire/synctools/icalendar/validation/FixInvalidDayOffsetPreprocessorTest.kt:10
- assertTrue is used below but not imported; add
import org.junit.Assert.assertTrueto ensure the tests compile.
import org.junit.Assert.assertFalse
lib/src/test/kotlin/at/bitfire/synctools/icalendar/validation/ICalPreprocessorTest.kt:35
- You're calling
mockkObjecton class instances rather than Kotlin objects; usemockkClass,mockkConstructor, or create a spy instance instead.
mockkObject(it)
lib/src/test/kotlin/at/bitfire/synctools/icalendar/validation/ICalPreprocessorTest.kt:41
- The
verifyblock wraps aforEach { it.preprocess(any()) }, which executes the loop inside the verification context instead of verifying each call; consider iterating and callingverify { it.preprocess(any()) }for each preprocessor.
verify {
sunkup
approved these changes
Jul 9, 2025
lib/src/test/kotlin/at/bitfire/synctools/icalendar/validation/ICalPreprocessorTest.kt
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Have used "injection" for
ICalendarParserTest. Even@InjectMockKsworks and injects the mockedICalPreprocessor.EventValidatoris intentionally not moved yet because it uses theEventdata class, which I'd like to get rid of in the new synctools (exception: legacy mappers).